Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

init simplification #3385

Merged
merged 2 commits into from
Aug 9, 2023
Merged

Conversation

kolyshkin
Copy link
Contributor

@kolyshkin kolyshkin commented Feb 19, 2022

Based on and currently includes #3375. Draft until that one is merged.
Based on and currently includes #3854. Draft until that one is merged.

This moves logging setup out of func init() and into StartInitialization() (where it should belong);

This concludes the spring cleaning started in #3354 (promise). Loosely based on parts of #3316.

@kolyshkin
Copy link
Contributor Author

Rebased on top of recently rebased #3375

@kolyshkin kolyshkin marked this pull request as ready for review March 27, 2022 01:40
@kolyshkin
Copy link
Contributor Author

Rebased; no longer a draft! Also, this concludes what was started as #3316

libcontainer/init_linux.go Outdated Show resolved Hide resolved
@kolyshkin
Copy link
Contributor Author

@opencontainers/runc-maintainers PTAL

@kolyshkin kolyshkin added this to the 1.2.0 milestone Apr 9, 2022
@kolyshkin
Copy link
Contributor Author

Rebased; @opencontainers/runc-maintainers PTAL (this concludes a very long and heavy refactoring, the new code is easier to follow and understand).

@kolyshkin
Copy link
Contributor Author

@thaJeztah @AkihiroSuda @cyphar PTAL

@kolyshkin kolyshkin force-pushed the init-logger-setup branch 2 times, most recently from 66de915 to 91d4bb0 Compare July 2, 2022 00:06
@thaJeztah
Copy link
Member

The first commit still has this paragraph in the commit message, which no longer appears to be the case;

Remove logrus setup and use from TestMain/init, in preparation for the
next commit.

libcontainer/factory_linux.go Outdated Show resolved Hide resolved
libcontainer/factory_linux.go Outdated Show resolved Hide resolved
libcontainer/init_linux.go Outdated Show resolved Hide resolved
libcontainer/init_linux.go Outdated Show resolved Hide resolved
libcontainer/init_linux.go Outdated Show resolved Hide resolved
@kolyshkin
Copy link
Contributor Author

The first commit still has this paragraph in the commit message, which no longer appears to be the case;

Remove logrus setup and use from TestMain/init, in preparation for the
next commit.

@thaJeztah why do you think so? The first commit removes the logrus setup from TestMain, and
it also removes logrus use from init() in the same file.

@kolyshkin kolyshkin changed the title Init logger setup Init logging cleanups/refactoring Aug 4, 2022
@kolyshkin kolyshkin marked this pull request as draft May 3, 2023 02:22
@kolyshkin
Copy link
Contributor Author

kolyshkin commented May 3, 2023

OK, maybe so many commits are way too much to digest. I have separated some prep commits to #3854, and made this one a draft. Once #3854 is merged, there will only be two commits to review here.

@kolyshkin kolyshkin force-pushed the init-logger-setup branch 2 times, most recently from 40e27c4 to 5aae844 Compare May 17, 2023 23:13
@kolyshkin kolyshkin marked this pull request as ready for review May 18, 2023 19:01
@kolyshkin kolyshkin changed the title Init logging cleanups/refactoring init simplification May 18, 2023
@@ -149,7 +161,7 @@ func StartInitialization() (retErr error) {
}()

// If init succeeds, it will not return, hence none of the defers will be called.
return containerInit(it, pipe, consoleSocket, fifofd, logPipeFd, mountFds)
return containerInit(it, pipe, consoleSocket, fifofd, logFD, mountFds)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, we now use 3 different ways to say "FD", but I'm pretty sure the last one is the best one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally prefer Fd. I can write up a PR to unify this.

@kolyshkin
Copy link
Contributor Author

@thaJeztah @AkihiroSuda PTAL

@AkihiroSuda
Copy link
Member

Needs rebase

Currently, logrus is used from the Go part of runc init, mostly for a
few debug messages (see setns_init_linux.go and standard_init_linux.go),
and a single warning (see rootfs_linux.go).

This means logrus is part of init implementation, and thus, its setup
belongs to StartInitialization().

Move the code there. As a nice side effect, now we don't have to convert
_LIBCONTAINER_LOGPIPE twice.

Note that since this initialization is now also called from libct/int
tests, which do not set _LIBCONTAINER_LOGLEVEL, let's make
_LIBCONTAINER_LOGLEVEL optional.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
This commit does two things:

1. Consolidate StartInitialization calling logic into Init().
2. Fix init error handling logic.

The main issues at hand are:
- the "unable to convert _LIBCONTAINER_INITPIPE" error from
  StartInitialization is never shown;
- errors from WriteSync and WriteJSON are never shown;
- the StartInit calling code is triplicated;
- using panic is questionable.

Generally, our goals are:
 - if there's any error, do our best to show it;
 - but only show each error once;
 - simplify the code, unify init implementations.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Copy link
Contributor Author

Rebased. @lifubang @thaJeztah PTAL

@kolyshkin kolyshkin merged commit cb44958 into opencontainers:main Aug 9, 2023
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants